Skip to content

Various developer improvements for shader compilation#35

Merged
doodlum merged 6 commits into
community-shaders:mainfrom
alandtse:main
Aug 7, 2023
Merged

Various developer improvements for shader compilation#35
doodlum merged 6 commits into
community-shaders:mainfrom
alandtse:main

Conversation

@alandtse
Copy link
Copy Markdown
Collaborator

Please note the change to menu open status. We definitely needed it in VR since our res is much smaller, but you may want to revert that or cherry pick the other changes.

alandtse and others added 6 commits July 29, 2023 00:17
Changes to a global define instead of per shader.
Add DeveloperMode and IsShaderEnabledAdded helper functions to identify
what shaders are enabled. Also clarifies developer mode is when log
level is trace or debug.
By default the features tab is the only meu that starts open. This
probably should turn into an INI option.
eyeOffset is no longer needed since we figured out the structure.

Fixes a reported shimmering in right eye.
@alandtse
Copy link
Copy Markdown
Collaborator Author

Note, I added a VR required fix to the end. Traveling so didn't have time to branch it off.

@TheRiverwoodModder
Copy link
Copy Markdown
Contributor

PR looks good, and it fixes the issue of the grass shader shimmering in the right eye.

@doodlum doodlum merged commit 84674fd into community-shaders:main Aug 7, 2023
alandtse referenced this pull request in alandtse/open-shaders Jul 20, 2025
Various developer improvements for shader compilation
SkrubbySkrubInAShrub pushed a commit that referenced this pull request May 24, 2026
input.WorldPosition in Effect.hlsl is camera-relative (the existing
fog and view-space transforms confirm this -- fog explicitly takes
CameraPosAdjust as a separate argument). GetShadowLightShadow's
ShadowProj is a world-space projection matrix, so passing camera-
relative coords yields shadow lookups offset by -CameraPosAdjust per
eye. Visible in VR (different offset per eye) and third-person.

Lighting.hlsl and RunGrass.hlsl already compute the world-space
position explicitly before calling GetShadowLightShadow; Effect.hlsl
now matches that pattern.

Caught by Copilot PR review on #35.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
SkrubbySkrubInAShrub pushed a commit that referenced this pull request May 24, 2026
Two issues caught by CodeRabbit on PR #35:

- Deferred::CopyShadowLightData was gated on
  ShadowCasterManager::GetInstalledSlotCount() != 0, which is the
  point/spot kSHADOWMAPS slice count. The sun lives in a separate
  target (kSHADOWMAPS_ESRAM), so the guard was incorrectly skipping
  sun directional shadow upload (t98/t99) whenever the point-shadow
  pool was empty. Remove the guard.

- LightLimitFix::Settings was missing EnableLightsVisualisation and
  LightsVisualisationMode from the NLOHMANN serialization macro, so
  those debug-overlay toggles reset on every restart. Add them to
  the macro. The LLFDEBUG permutation toggle at startup is a deeper
  plumbing change deferred for a follow-up.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
SkrubbySkrubInAShrub pushed a commit that referenced this pull request May 26, 2026
Address 7 actionable Copilot review comments on PR #35:

- CMakeLists.txt: mark `find_path(EXPRTK_INCLUDE_DIRS)` REQUIRED. Without
  it, a missing exprtk silently propagates `-NOTFOUND` into
  target_include_directories and the build fails later with a confusing
  diagnostic instead of at configure time.

- ShadowCasterManager.h: correct GetShadowSlot's docstring. The previous
  text claimed "0 = sun, 1+ = point lights" but the implementation
  returns -1 for both the sun and inactive lights, and 1..N for points.
  The sun lives in kSHADOWMAPS_ESRAM (separate texture from kSHADOWMAPS),
  so it has no slot in the point-light array.

- LightLimitFix.cpp: extend NLOHMANN serialization to include
  EnableContactShadows, EnableLightsVisualisation, and
  LightsVisualisationMode. These debug/UI fields previously didn't
  persist across sessions even though they have JSON-loadable defaults.

- LightLimitFix.hlsli: bounds-check shadowIndex against ShadowMapSlots
  in GetShadowLightShadow before indexing the Shadows StructuredBuffer.
  Overflowed slots (modelled explicitly elsewhere in the PR) would
  otherwise read invalid records or slices; the new guard falls back to
  "unshadowed + no coverage" cleanly.

- ShadowRenderer.cpp: when GetInstalledSlotCount() returns 0, clear
  per-frame slot metadata, counters, and PS bindings at t100/t101
  before early-returning. Previously the stale state lingered across
  zero-slot frames -- the overlay kept showing the previous frame's
  shadow rows and shaders kept sampling the previous frame's records.

- LightLimitFix.cpp: validate shadowmapIndex against
  ShadowCasterManager::GetInstalledSlotCount before setting
  LightFlags::Shadow in BSLightingShader_SetupGeometry. A stale or
  corrupted descriptor mid-frame could otherwise carry an out-of-range
  index and the shader's Shadows[] / ShadowMaps[] reads would sample
  OOB (UB, GPU crash on some drivers).

Three threads resolved without code changes:
- Effect.hlsl GetShadowLightShadow world-space arg: already passes the
  camera-relative -> world-space conversion. Outdated against an earlier
  revision of the file.
- ShadowRenderer.cpp <chrono> include: the file no longer uses
  std::chrono (refactored to QPC per project convention).
- Globals.cpp engine-global pointers: critical relocations via 2-arg
  RelocationID intentionally abort at boot on failure rather than
  silently corrupting -- defense-in-depth wrapper would mask real load
  failures, not catch them.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
SkrubbySkrubInAShrub pushed a commit that referenced this pull request May 26, 2026
The shadowmapIndex bounds check added in the PR #35 review-feedback
commit gated `LightFlags::Shadow` on `idx > 0 && idx < installedSlots`.
The `> 0` lower bound was overreach -- slot 0 of kSHADOWMAPS is a
legitimate slice for a point/spot light when the sun isn't in the SCM
pool (per GetShadowSlot's behavior: pool index maps 1:1 to kSHADOWMAPS
slot, and the sun's pool slot returns the -1 sentinel, freeing slot 0
for actual point lights). Excluding slot 0 silently dropped shadow
rendering for any light occupying that slice -- effectively reverting
a working code path.

The review comment only flagged the upper-bound OOB risk (sampling
past the allocated slice count). Keep that check; drop `> 0`.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
SkrubbySkrubInAShrub pushed a commit that referenced this pull request May 26, 2026
Three more Copilot review comments on PR #35 after the initial fix
push:

- Effect.hlsl: gate strict lights behind `inWorld` the same way
  clustered lights are gated. Previously totalLightCount included
  NumStrictLights regardless of inWorld, which meant non-world effect
  passes (UI, blood splatter on screen-space surfaces, etc.) would
  iterate strict lights from the world-space CB and pick up unrelated
  lighting. The clustered branch was already gated; strict matches
  for symmetry now.

- ShadowRenderer.cpp: replace per-frame `std::vector<ShadowLightData>
  sd(slots)` ctor with a `static` vector + `assign(slots, {})` to
  reuse the backing storage. The previous shape heap-allocated every
  frame in CopyShadowLightData's hot path; slot count is stable
  outside resolution / settings reconfigures so reusing storage is
  free win.

- LightLimitFix.cpp: `shadowLightPtrs.reserve(GetInstalledSlotCount()
  + 1)` before the ForEachShadowLight insert loop. The unordered_set
  was constructed empty and grew via inserts each frame, causing
  multiple rehash allocations under high shadow counts. Upper bound
  is the configured kSHADOWMAPS slot count (+1 belt-and-braces for
  the sun's logical entry).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
SkrubbySkrubInAShrub pushed a commit that referenced this pull request May 26, 2026
Three more Copilot review comments on PR #35:

- Deferred.cpp focus shadow upload: preserve descriptor->slice
  correspondence by writing FocusShadowProj[i] for descriptor[i]
  instead of compacting densely via dd.FocusShadowCount. The LLF
  shader samples kSHADOWMAPS slice (4 + fi) using fi as the matrix
  index, so packing past disabled holes would pair matrix N with the
  wrong shadow slice. Disabled descriptors now leave their slot at
  the default-zero matrix; the shader's existing
  `focusClip.w <= EPSILON_DIVISION continue` guard treats zero
  matrices as "no actor in this slice" and skips sampling.
  FocusShadowCount is the upper iteration bound (last enabled
  index + 1).

- LightLimitFix.cpp BSLightingShader_SetupGeometry strict-light
  setup: read the kSHADOWMAPS slice from
  ShadowCasterManager::GetShadowSlot() instead of
  shadowmapDescriptors[0].shadowmapIndex. The descriptor field can
  be corrupted mid-frame by ReturnShadowmaps() (called via
  Hook_DisableColorMask) after ScheduleShadowCasters fixed it but
  before this strict-light setup runs -- a stale-but-in-range index
  would still pass the upper-bound check yet point strict-light
  shader sampling at the wrong slice. GetShadowSlot reads from the
  SCM's stable s_lights pool (set in ScheduleShadowCasters, never
  touched by ReturnShadowmaps), aligning with how
  CopyShadowLightData and UpdateLights already key off it.

- ShadowCasterManager.h GetShadowSlot docstring: corrected the
  advertised range from "1..ShadowLightCount-1" to the actual
  "0..GetInstalledSlotCount()-1" raw kSHADOWMAPS slice. The previous
  text was off by one and would mislead a future caller into
  skipping slice 0 or adding bogus bias logic.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
SkrubbySkrubInAShrub pushed a commit that referenced this pull request May 26, 2026
Two real findings from the Copilot review pass, the rest either already
addressed in earlier review rounds or intentionally not-fixed with
documented rationale.

1. FormulaHelper::Parse leaked state on compile failure. The previous
   sequence assigned `_ptr = new FormulaWrapper()` BEFORE calling
   parser.compile(), so a failed compile left the helper in a "parsed"
   state: subsequent Parse() calls returned false via the early
   `if (_ptr) return false` guard, and Calculate() would evaluate the
   uncompiled wrapper. Defer the _ptr assignment until compile succeeds,
   and delete the wrapper on failure.

2. ShadowBitMask was dead work. The field is declared in the LLF cbuffer
   struct but no shader reads it (the bit-mask-based IsLightIgnored
   branch was removed when per-light shadowMapIndex sampling replaced
   it). The C++ side was building it in a per-pass hot loop over
   numShadowLights and using it in the change-detect that drives
   strictLightDataCB->Update(). Drop the build loop and remove the
   change-detect comparison; the field stays initialized to 0 above for
   cbuffer ABI stability. Also removes the previousShadowBitMask member.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
SkrubbySkrubInAShrub pushed a commit that referenced this pull request May 26, 2026
PR #35 CI surfaced 360 new X4000 "use of potentially uninitialized
variable" warnings, all from LightLimitFix.hlsli:396 (the closing brace
of the shadowIndex>=ShadowMapSlots overflow guard).

The previous pattern was:
    hasCoverage = true;
    if (overflow) { hasCoverage = false; return 1.0; }
    ... rest sets nothing, all returns leave hasCoverage = true.
Semantically correct -- the entry assignment dominates -- but FXC's
dataflow analysis flagged the post-if merge as a potential
uninitialized read on the `out bool hasCoverage` parameter.

Restructure so each return path writes hasCoverage exactly once. The
overflow branch writes false; everything past it writes true (paraboloid
lights always sample, including the safe / suppressed sentinels). The
0 new warnings limit is restored.

Also initialise the caller's `bool shadowCoverage` in Effect.hlsl for
parity with the other call sites (Lighting.hlsl, RunGrass.hlsl x2),
which all use `= false`. Defensive only; FXC doesn't flag the caller.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants